Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

🐛 Fix AttributeError in datasets with missing metadata parameter (#67) #68

Merged
merged 1 commit into from
May 28, 2024
Merged

🐛 Fix AttributeError in datasets with missing metadata parameter (#67) #68

merged 1 commit into from
May 28, 2024

Conversation

michal-mmm
Copy link
Contributor

Description

Why was this PR created?
Fix #67

Development notes

What have you changed, and how has this been tested?

  • added getattr() with None as a default value for accessing metadata

Checklist

  • Read the contributing guidelines
  • Open this PR as a 'Draft Pull Request' if it is work-in-progress
  • Update the documentation to reflect the code changes
  • Add a description of this change and add your name to the list of supporting contributions in the CHANGELOG.md file. Please respect Keep a Changelog guidelines.
  • Add tests to cover your changes

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

  • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.

  • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.

  • I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

@Galileo-Galilei
Copy link
Owner

Hi, thank you very much for the PR. Can you please:

  • rebase on master and move the fix to the new _validate_dataset method?
  • Add an entry to the CHANGELOG.md?

I'll release as soon as you push.

@michal-mmm
Copy link
Contributor Author

Hi, I've made the changes. But now one test locally is failing:
FAILED tests/framework/hooks/test_hook.py::test_validate_only_once - assert 2 == 1
It's due to a previous PR (#66)

@michal-mmm
Copy link
Contributor Author

What do you think about adding this fragment? Remove logging info? Or make it a separate PR?

for name, data in datasets.items(): # not changed
   if name in self._validated_datasets:
      self._logger.info(f"(kedro-pandera) Dataset '{name}' has been already validated with pandera")
      continue
   metadata = getattr(catalog._datasets[name], "metadata", None) # not changed

@Galileo-Galilei
Copy link
Owner

It's due to the removal of line 61 in the if condition: and name not in self._validated_datasets, you should put it back

@codecov Codecov
Copy link

codecov bot commented May 28, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (dc12425) to head (8cdb17c).
Report is 8 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff            @@
##              main       #68   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          112       113    +1     
=========================================
+ Hits           112       113    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@michal-mmm
Copy link
Contributor Author

you're right, I'm blind :-) I've added the missing condition

@Galileo-Galilei Galileo-Galilei merged commit e40fd99 into Galileo-Galilei:main May 28, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AttributeError due to missing metadata parameter in datasets
2 participants
@michal-mmm @Galileo-Galilei